The basic code structure is that we have generic blocks in Mage_Eav that make grids and edit forms for any EAV attribute. To determine which one we are editing, we read from the registry.

Then we also have an abstract controllers that take care of the grid/edit/save/etc actions. For each EAV attribute we want to allow the user to edit, we just add the menu item and extend from the abstract controllers.

Also for discussion, what is the reason someone would edit any of the Sales (Orders, Invoice, Creditmemo, Shipment) EAV attributes, or attribute sets? Has anyone ever done anything with these EAV entities in custom code? Would we want to show extra attributes in admin create order screens?

Also for discussion, what is the reason someone would edit any of the Sales (Orders, Invoice, Creditmemo, Shipment) EAV attributes, or attribute sets? Has anyone ever done anything with these EAV entities in custom code? Would we want to show extra attributes in admin create order screens?

I did that a lot of times in the past 12 years :-D
but only attributes and not sets, for those entities

I love this PR but I think there's a problem, orders/invoice etc have flat tables with static attributes (meaning columns in the flat table), not in the EAV. If I add an order attribute I want to have it on the flat table (maybe even on the _grid table) but not in the EAV...

I also don't know how much sense would the attribute_set feature have, since they're _flat tables... same thing for the "scope".

Same thing for the customer attribute sets, I'm not sure to force user to choose an attribute set for every customer group.. I'd leave that part for a future implementation (and that's why I didn't include it in my original PRs)

Also for discussion, what is the reason someone would edit any of the Sales (Orders, Invoice, Creditmemo, Shipment) EAV attributes, or attribute sets? Has anyone ever done anything with these EAV entities in custom code? Would we want to show extra attributes in admin create order screens?

I did that a lot of times in the past 12 years :-D but only attributes and not sets, for those entities

I love this PR but I think there's a problem, orders/invoice etc have flat tables with static attributes (meaning columns in the flat table), not in the EAV. If I add an order attribute I want to have it on the flat table (maybe even on the _grid table) but not in the EAV...

I also don't know how much sense would the attribute_set feature have, since they're _flat tables... same thing for the "scope".

I add columns to the flat table quite often as well, I just wasn't thinking of it as using EAV attributes, but I guess it is technically? Did Magento used to use the EAV table for orders before they decided on the flat tables for performance?

Anyway, so should we just remove all Sales > Attributes menu for now? It's as easy as dropping that one commit.

Same thing for the customer attribute sets, I'm not sure to force user to choose an attribute set for every customer group.. I'd leave that part for a future implementation (and that's why I didn't include it in my original PRs)

I was thinking similar @kiatng's comment in the other PR:

I have a similar custom extension, including the Manage Attribute Sets for organizing the attributes for different customer groups.

Where customer groups would by default use the default attribute set, so the user would only need to change it if they wanted to. But more than that, the custom attributes do not even show up anywhere in the Customer > Edit screen yet, we need the use_in_forms field controllable.

All sales entities do not use EAV. There is no trace of it except in the table eav_entity_type:

I think the last 4 rows were forgotten and are now ghosts.(It's used for increment_id in table eav_entity_store.) The collections of the sales entities do not extend the EAV class, it has its own abstract class for BC to EAV:

So, only the catalog and customer module use EAV. Recently, I have implemented a custom EAV module. So, for me, I can see a need to abstract common functions to the EAV module instead of extending to the catalog module, and as pointed out by @fballiano, create unnecessary dependency.

I use attribute sets to organize the attributes for different customer groups. The project has over 200 customer attributes, each customer group having very different set of attributes.

My concern is that changes to EAV could make it very difficult to merge OM to existing projects with custom EAV.

So if the only EAV entities are actually customer, customer_address, catalog and category maybe my approach wasn't so bad after all :-D

but... somebody could have custom EAV entities and it would be awesome to manage their attributes with a GUI anyway...

so maybe we could remove the 4 non-EAV entities from this PR and postpone it after merging this one?

So let's definitely just remove the catalog EAV grids. Technically you could add attributes & sets for orders, invoices, etc, but with no way to retrieve those values in the collections it's pointless and confusing.

I use attribute sets to organize the attributes for different customer groups. The project has over 200 customer attributes, each customer group having very different set of attributes.

My concern is that changes to EAV could make it very difficult to merge OM to existing projects with custom EAV.

The PR as it now should definitely not conflict with anything since it's all new code, but I see your point if we hook up a customer group -> customer attribute set relation. Still, I think it could be worth it maybe just waiting to merge this PR until the next major version.

How did you correlate attribute sets with customer groups? My first thought is to modify the customer_group table:

mysql> describe customer_group;
+---------------------+-------------------+------+-----+---------+----------------+
| Field               | Type              | Null | Key | Default | Extra          |
+---------------------+-------------------+------+-----+---------+----------------+
| customer_group_id   | smallint unsigned | NO   | PRI | NULL    | auto_increment |
| customer_group_code | varchar(32)       | NO   |     | NULL    |                |
| tax_class_id        | int unsigned      | NO   |     | 0       |                |
+---------------------+-------------------+------+-----+---------+----------------+
3 rows in set (0.00 sec)

To add something like customer_attribute_set_id and maybe even customer_address_attribute_set_id. Probably both are useful in the case you want to control both attribute sets per group.

If we can do it in a way that is maybe compatible with what you've done, then that's at least one site that wouldn't break.

Okay so next question, is about attribute sets on categories. I can't see a need for this, I just included it in the PR since it was only a few lines to do so. How should the menu entries look like? A few options:

• Catalog

  • Manage Products
  • Manage Categories
  • Attributes
    • Product
      • Attributes
      • Attribute Sets
    • Category
      • Attributes

• Catalog

  • Manage Products
  • Manage Categories
  • Attributes
    • Product Attributes
    • Product Attribute Sets
    • Category Attributes

• Catalog

  • Manage Products
    • Attributes
    • Attribute Sets
  • Manage Categories
    • Attributes
  • Attributes

Option 1 is like I have it now in this PR, just with the Category Attribute Sets option removed. Option 2 is like @fballiano had it but with the word "Product" added to the first two options. Option 3 is a bit weird because I don't think there's other cases where a clickable menu item has children.

So if the only EAV entities are actually customer, customer_address, catalog and category maybe my approach wasn't so bad after all :-D

but... somebody could have custom EAV entities and it would be awesome to manage their attributes with a GUI anyway...

so maybe we could remove the 4 non-EAV entities from this PR and postpone it after merging this one?

Yeah, your original PRs were definitely on the money here. I just thought it could be abstracted and then I added all the EAV entities since it only requires adding the controller & adminhtml.xml entries.

I definitely would consider now to make my own EAV entities though, since the GUI boilerplate is taken care of.

I reverted the catalog grids in a commit, I am not sure what you mean by the second part (postpone what?)

P.S. I will squash and force push some of these commits before this PR is merged (if it is merged). I just didn't want to keep force-pushing in case anyone has cloned this branch.

Okay so next question, is about attribute sets on categories. I can't see a need for this, I just included it in the PR since it was only a few lines to do so. How should the menu entries look like? A few options:

I category_attribute_set should be tied to the store, since you may want to have different attribute sets for categories in different websites (but I'd use store, not store_views, since the store is the root category).

Option 1 is like I have it now in this PR, just with the Category Attribute Sets option removed. Option 2 is like @fballiano had it but with the word "Product" added to the first two options. Option 3 is a bit weird because I don't think there's other cases where a clickable menu item has children.

I wouldn't change the products attribute admin path that everybody has now in production, since we'd need an update script to fix the ACL. So I think I'd go for option 2.

I reverted the catalog grids in a commit, I am not sure what you mean by the second part (postpone what?)

I mean that this PR shouldn't do anything about order/quote/invoice etc

Okay so next question, is about attribute sets on categories. I can't see a need for this, I just included it in the PR since it was only a few lines to do so. How should the menu entries look like? A few options:

I category_attribute_set should be tied to the store, since you may want to have different attribute sets for categories in different websites (but I'd use store, not store_views, since the store is the root category).

+1, that makes sense to me. Is it too much to add in to this PR the code to associate category attribute sets to stores, and customer/customer address attribute sets to customer groups? Should that be a follow up PR?

Option 1 is like I have it now in this PR, just with the Category Attribute Sets option removed. Option 2 is like @fballiano had it but with the word "Product" added to the first two options. Option 3 is a bit weird because I don't think there's other cases where a clickable menu item has children.

I wouldn't change the products attribute admin path that everybody has now in production, since we'd need an update script to fix the ACL. So I think I'd go for option 2.

If we allow the Category Attribute Sets option, then I am leaning towards this ideally:

• Catalog
  • Manage Products
  • Manage Categories
  • Attributes
    • Product
      • Attributes
      • Attribute Sets
    • Category
      • Attributes
      • Attribute Sets

To fix the BC break, I made PR #2321 that should allow us to change the ACL & menu structure without changing the ACL path.

+1, that makes sense to me. Is it too much to add in to this PR the code to associate category attribute sets to stores, and customer/customer address attribute sets to customer groups? Should that be a follow up PR?

I'd do it in a separate PR cause I think it's not everybody's use case and one may want to add/edit attributes and write custom code to handle them on the frontend side

• Catalog

  • Manage Products

  • Manage Categories

  • Attributes

    • Product

      • Attributes
      • Attribute Sets

    • Category

      • Attributes
      • Attribute Sets

To me it's not super necessary to change the path and introduce that other PR to handle that.
I'd simpy do

• catalog
• • manage products
• • manage categories
• • attributes
• • • manage product attributes
• • • manage product attribute sets
• • • manage category attributes
• • • manage cateogyr attribute sets

it's never extremely comfortable to navigate many sublevels plus there would be only 4 items in the "attributes" one, not a very messy situation ;-)

I'd do it in a separate PR cause I think it's not everybody's use case and one may want to add/edit attributes and write custom code to handle them on the frontend side

Yes, sounds good.

it's never extremely comfortable to navigate many sublevels plus there would be only 4 items in the "attributes" one, not a very messy situation ;-)

Okay, I agree with that. The other reason for PR #2321 was to allow setting a custom ACL path to admin/eav/$ENTITY/attributes and admin/eav/$ENTITY/sets for the other admin pages, but I realize now since that check is in the controller that we extend, we can just define our own _isAllowed method in those classes (i.e. in app/code/core/Mage/Adminhtml/controllers/Catalog/Category/AttributeController.php)

I pushed the menu changes. A few other TODOs:

Block the user from editing/deleting "system" attributes

Should we do this? It seems like we are not blocked from editing system product attributes. I suppose an admin could probably do some damage by editing those items, but I do not know what will happen.

Review the SQL commands at the end of this comment: #2264 (comment)

UPDATE eav_attribute SET frontend_input = 'boolean' WHERE source_model = 'eav/entity_attribute_source_boolean' AND entity_type_id = 3;

DELETE eag.* FROM eav_attribute_set eas LEFT JOIN eav_attribute_group eag ON eas.attribute_set_id = eag.attribute_set_id WHERE eas.entity_type_id = 3 AND eag.attribute_group_name = 'General';
UPDATE eav_attribute_set eas LEFT JOIN eav_attribute_group eag ON eas.attribute_set_id = eag.attribute_set_id SET eag.default_id = 1 WHERE eas.entity_type_id = 3 AND eag.attribute_group_name = 'General Information';

I think it's a bug that we had frontend_input = 'boolean' but not source_model = 'eav/entity_attribute_source_boolean'. So if you went to edit some of the system attributes (which you should probably not do) then you could add options to a yesno type. That obviously makes no sense.

Also, we can clean up some the unused attribute group name "General" for the default product category attribute set.

I appreciate the feedback and of course your original initiative on this @fballiano, I do not want to let this PR stagnate so I'm trying to work through all the issues. :)

it's possible to edit system attributes but not delete them, I think we should have the same policy here.

Although... let's take "media_gallery" attribute as an example... I can't imagine what's happening if you modify the "scope" of this attribute but it's been like this since the beginning of magento so...

About the SQL query review, I couldn't test it but maybe it should be a PR on its own?

@fballiano It shouldn't be possible right now to delete system attributes because of this line:

I don't know what would happen either if someone totally messes up system attributes by changing the scope or other properties of the attribute. But since it's possible to edit system attributes for the catalog_product entity, I think any changes preventing that should be in another PR.

I'll knock a few other TODOs from my list, squash & force push this branch to clean it up some.

@justinbeaty is it okay for you to merge that other two PRs into - to make it testable?

@justinbeaty is it okay for you to merge that other two PRs into - to make it testable?

In hindsight, having three PRs did in fact make this too complicated. It would be easier for me to port MahoCommerce/maho#25 back to OpenMage, but you would need to re-run phpcs, rector, etc.

developing this thing is 2 different projects is beyond nonsense.

developing this thing is 2 different projects is beyond nonsense.

I don't disagree, it will probably be easier to backport it once it's merged in Maho.

it's nice to see that bringing developing something in Maho makes OM wants to merge it immediately after it was left dead for 2 years.

competitions makes things better ;-)

let me know if you want to finish this, I will not waste time because you want to race and will merge the PR once it's finished. if that's how it has to go.

I would recommend a separate public discussion. This new e-commerce project named Maho relies on OpenMage platform and not vice versa. The big problem is that Magento 1 didn't offer the 3 PRs out of the box and we had to wait 16 years to discuss them. My opinion is that we should finish them in OpenMage first and then any fork to benefit from it. As far as I am concerned, I choose to contribute to a project where I am not blocked by the owner.

I would recommend a separate public discussion. This new e-commerce project named Maho relies on OpenMage platform and not vice versa. The big problem is that Magento 1 didn't offer the 3 PRs out of the box and we had to wait 16 years to discuss them. My opinion is that we should finish them in OpenMage first and then any fork to benefit from it. As far as I am concerned, I choose to contribute to a project where I am not blocked by the owner.

Since the EAV editor was originally @fballiano's idea, I am deciding to contribute the PR to Maho. He is right that it is too much effort to finish developing this feature, bug squashing, etc on two repos. But since this is all Open Source Software, it can be merged here later, or you can continue with the PRs we already have open to OM. I will not be deleting my OM repo that this PR came from.

@justinbeaty - The choice is yours. For me it is less important where it is baked, I hope to see functional in OpenMage in the next months.

I would recommend a separate public discussion.

this is specifically about this PR so why should it be discussed in another place? anyway there's no discussion to make. so you can delete and lock the discussion as it became usual on this repo.
you have to decide if you want to finish this PR or, as for sure will happen, you will wait for me to finish it, then do the same only to create an incompatibility between the projects.

This new e-commerce project named Maho relies on OpenMage platform and not vice versa.

OM has 480 commits made by me (only on the main branch, let's not even calculate the other branches) so don't patronize me on who depends on who. if somebody knows about openmage in 2024 it's only because I spent 3.5 years doing public relations for it, not the other way around.

My opinion is that we should finish them in OpenMage first and then any fork to benefit from it.

again, if you want to finish them then do that, but doing the ego race, no thanks, I've 1000 other ideas to develop and opensource is great because, respecting licenses, everybody can benefit.

As far as I am concerned, I choose to contribute to a project where I am not blocked by the owner.

that's quite obvious, you can't do otherwise anyway